Replace pullRealmFiles with BoxelCLIClient.pull()#4422
Replace pullRealmFiles with BoxelCLIClient.pull()#4422jurgenwerk wants to merge 1 commit intomainfrom
Conversation
Add a programmatic pull() method to BoxelCLIClient that reuses the
existing RealmPuller (auth via active profile, concurrent downloads,
checkpoints) and returns { files, error? } instead of calling
process.exit().
Replace the 90-line HTTP implementation of pullRealmFiles in
realm-operations.ts with a 3-line delegation to the new client method.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
jurgenwerk
changed the title
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 780a313c55
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| _options?: RealmFetchOptions, | ||
| ): Promise<{ files: string[]; error?: string }> { | ||
| let fetchImpl = options?.fetch ?? globalThis.fetch; | ||
| let normalizedRealmUrl = ensureTrailingSlash(realmUrl); | ||
|
|
||
| let headers = buildAuthHeaders( | ||
| options?.authorization, | ||
| SupportedMimeType.JSONAPI, | ||
| ); | ||
|
|
||
| // Fetch mtimes to discover all file paths. | ||
| let mtimesUrl = `${normalizedRealmUrl}_mtimes`; | ||
| let mtimesResponse: Response; | ||
| try { | ||
| mtimesResponse = await fetchImpl(mtimesUrl, { method: 'GET', headers }); | ||
| } catch (err) { | ||
| return { | ||
| files: [], | ||
| error: `Failed to fetch _mtimes: ${err instanceof Error ? err.message : String(err)}`, | ||
| }; | ||
| } | ||
|
|
||
| if (!mtimesResponse.ok) { | ||
| let body = await mtimesResponse.text(); | ||
| return { | ||
| files: [], | ||
| error: `_mtimes returned HTTP ${mtimesResponse.status}: ${body.slice(0, 300)}`, | ||
| }; | ||
| } | ||
|
|
||
| let mtimes: Record<string, number>; | ||
| try { | ||
| let json = await mtimesResponse.json(); | ||
| // _mtimes returns JSON:API format: { data: { attributes: { mtimes: {...} } } } | ||
| mtimes = | ||
| (json as { data?: { attributes?: { mtimes?: Record<string, number> } } }) | ||
| ?.data?.attributes?.mtimes ?? json; | ||
| } catch { | ||
| return { files: [], error: 'Failed to parse _mtimes response as JSON' }; | ||
| } | ||
|
|
||
| // Download each file. | ||
| let downloadedFiles: string[] = []; | ||
| for (let fullUrl of Object.keys(mtimes)) { | ||
| if (!fullUrl.startsWith(normalizedRealmUrl)) { | ||
| continue; | ||
| } | ||
| let relativePath = fullUrl.slice(normalizedRealmUrl.length); | ||
| if (!relativePath || relativePath.endsWith('/')) { | ||
| continue; | ||
| } | ||
|
|
||
| let localPath = join(localDir, relativePath); | ||
| mkdirSync(dirname(localPath), { recursive: true }); | ||
|
|
||
| try { | ||
| let fileResponse = await fetchImpl(fullUrl, { | ||
| method: 'GET', | ||
| headers: buildAuthHeaders( | ||
| options?.authorization, | ||
| SupportedMimeType.CardSource, | ||
| ), | ||
| }); | ||
|
|
||
| if (!fileResponse.ok) { | ||
| continue; | ||
| } | ||
|
|
||
| let rawText = await fileResponse.text(); | ||
| writeFileSync(localPath, rawText); | ||
| downloadedFiles.push(relativePath); | ||
| } catch { | ||
| continue; | ||
| } | ||
| } | ||
|
|
||
| return { files: downloadedFiles.sort() }; | ||
| let client = new BoxelCLIClient(); | ||
| return client.pull(realmUrl, localDir); |
There was a problem hiding this comment.
pullRealmFiles still accepts RealmFetchOptions, but the new delegation ignores _options entirely and always uses a fresh BoxelCLIClient profile. Any caller that passes authorization (or a mocked fetch for controlled execution) now gets silently different behavior—most critically, private realm pulls can fail with "No active profile" even when a valid bearer token was provided.
Useful? React with 👍 / 👎.
| { | ||
| realmUrl, | ||
| localDir, | ||
| deleteLocal: options.delete, | ||
| }, |
There was a problem hiding this comment.
The new exported pull() API accepts PullCommandOptions (which includes dryRun), but it does not pass dryRun into RealmPuller. As a result, pull(..., { dryRun: true }) will still write files and perform real mutations, which violates the expected no-side-effects behavior used by the CLI path.
Useful? React with 👍 / 👎.
| } | ||
| } | ||
|
|
||
| export async function pull( |
There was a problem hiding this comment.
I think we can drop the pullCommand function and use this function in registerPullCommand and move the console.log and process.exit that previously handled in pullCommand to registerPullCommand.
There was a problem hiding this comment.
Pull request overview
This PR refactors pullRealmFiles in software-factory to delegate realm downloading to a new programmatic BoxelCLIClient.pull() API that reuses boxel-cli’s existing RealmPuller behavior, and removes software-factory unit tests that were tied to the old HTTP implementation.
Changes:
- Added
BoxelCLIClient.pull()plus exportedPullOptions/PullResulttypes in@cardstack/boxel-cli/api. - Added a programmatic
pull()function to boxel-cli’s realm pull command module that returns{ files, error? }instead of callingprocess.exit(). - Replaced the previous HTTP-based
pullRealmFilesimplementation with a delegation toBoxelCLIClient.pull()and removed associated software-factory tests.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/software-factory/tests/factory-test-realm.test.ts | Removes unit tests that validated the old HTTP-based pull behavior. |
| packages/software-factory/src/realm-operations.ts | Replaces the previous _mtimes/HTTP download implementation with a BoxelCLIClient.pull() delegation. |
| packages/boxel-cli/src/lib/boxel-cli-client.ts | Adds pull() to the programmatic client API and defines PullOptions/PullResult. |
| packages/boxel-cli/src/commands/realm/pull.ts | Tracks downloaded files on RealmPuller and adds a new pull() function returning { files, error? }. |
| packages/boxel-cli/api.ts | Exports the new pull-related option/result types. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| { | ||
| realmUrl, | ||
| localDir, | ||
| deleteLocal: options.delete, |
There was a problem hiding this comment.
pull() accepts PullCommandOptions (including dryRun), but it does not pass options.dryRun into RealmPuller. As written, a caller providing { dryRun: true } would still perform real filesystem writes/checkpoints.
Pass dryRun: options.dryRun when constructing RealmPuller (or remove dryRun from this API if it’s intentionally unsupported).
| deleteLocal: options.delete, | |
| deleteLocal: options.delete, | |
| dryRun: options.dryRun, |
| export async function pull( | ||
| realmUrl: string, | ||
| localDir: string, | ||
| options: PullCommandOptions, | ||
| ): Promise<{ files: string[]; error?: string }> { | ||
| let pm = options.profileManager ?? getProfileManager(); | ||
| let active = pm.getActiveProfile(); | ||
| if (!active) { | ||
| return { | ||
| files: [], | ||
| error: 'No active profile. Run `boxel profile add` to create one.', | ||
| }; |
There was a problem hiding this comment.
New programmatic pull() is introduced to return { files, error? } instead of calling process.exit(), but existing integration coverage in this repo appears to exercise pullCommand() only. Add a test that calls pull() directly (and/or BoxelCLIClient.pull()) to verify it returns an error string on failure conditions (e.g., no active profile / partial download errors) and does not terminate the process.
| localDir: string, | ||
| options?: RealmFetchOptions, | ||
| _options?: RealmFetchOptions, | ||
| ): Promise<{ files: string[]; error?: string }> { |
There was a problem hiding this comment.
pullRealmFiles() keeps the RealmFetchOptions parameter but now ignores it entirely (including authorization and injected fetch). This is a silent behavior change for any caller that still passes these options.
Consider either (a) updating the signature/type to match the new behavior (e.g. a PullOptions that maps to boxel-cli), or (b) explicitly returning an error when _options contains unsupported fields so callers don’t think auth injection is still honored.
| ): Promise<{ files: string[]; error?: string }> { | |
| ): Promise<{ files: string[]; error?: string }> { | |
| if (_options && Object.keys(_options).length > 0) { | |
| return { | |
| files: [], | |
| error: | |
| 'pullRealmFiles no longer supports RealmFetchOptions such as authorization or injected fetch; it now uses boxel-cli pull with active-profile authentication.', | |
| }; | |
| } |
| }); | ||
| }); | ||
| // pullRealmFiles tests removed — pullRealmFiles now delegates to | ||
| // BoxelCLIClient.pull() which is tested in the boxel-cli package. |
There was a problem hiding this comment.
This comment says BoxelCLIClient.pull() is tested in the boxel-cli package, but the current test suite appears to cover pullCommand() (integration) rather than the BoxelCLIClient wrapper or the new programmatic pull() return-value API. Consider rewording this to match what’s actually covered, or add a targeted test for the client/pull wrapper so the statement stays true.
| // BoxelCLIClient.pull() which is tested in the boxel-cli package. | |
| // BoxelCLIClient.pull(), and pull behavior is covered in the boxel-cli | |
| // package. |
There was a problem hiding this comment.
make sure to address the co-pilot issues, but i think it looks good. it puts us in a good place to tackle https://linear.app/cardstack/issue/CS-10520/software-factory-delegates-all-boxel-api-calls-to-boxel-cli
Summary
pull()method toBoxelCLIClientthat reuses the existingRealmPuller(auth via active profile, concurrent downloads, checkpoints) and returns{ files, error? }instead of callingprocess.exit()pullRealmFilesinrealm-operations.tswith a 3-line delegation to the new client methodTest plan
pullRealmFilesdelegation🤖 Generated with Claude Code